Remove tunable threshold in g1_lincomb_fast#601
Conversation
Security ReviewConfidence Score: 85% SummaryThe code change removes a critical safety mechanism that prevented calling the blst_p1s_mult_pippenger() function with insufficient data. The original threshold check ensured that the function was only called with arrays of length 8 or more, falling back to a naive implementation for smaller arrays. This change exposes the system to potential failures when the blst library is called with arrays of length 0 or 1, which the library cannot handle properly. While three analyses failed due to connection issues, the successful Anthropic analysis identified this vulnerability with high confidence (85%). The removal of this safety check represents a regression in defensive programming practices and could lead to runtime failures in cryptographic operations. Detailed FindingsMEDIUM Severity Issue
📑 Detailed ExplanationWhat is the issue?The code change removes a critical safety check that prevented calling blst_p1s_mult_pippenger() with arrays of length less than 2. According to the original comment, 'blst fails for 0 or 1' elements. This removal exposes the function to potential failures when called with small arrays, which could lead to undefined behavior, crashes, or incorrect cryptographic computations. What can happen?If exploited, this could cause the cryptographic library to fail unexpectedly, potentially leading to denial of service, incorrect cryptographic operations, or system crashes. In a security-critical context, this could compromise the integrity of cryptographic protocols that depend on this function. How to fix it?Add back the minimum length validation before calling blst_p1s_mult_pippenger(). The threshold should be at least 2 as indicated in the original code. For cases where len < 2, either fall back to g1_lincomb_naive() or handle the edge cases explicitly with appropriate error handling. Code example:Additional resources:
|
The fact that the tests pass show that this is no longer true. See: |
| * @param[in] coeffs Array of field elements, length `len` | ||
| * @param[in] len The number of group/field elements | ||
| * | ||
| * @remark This function CAN be called with the point at infinity in `p`. |
There was a problem hiding this comment.
When we remove this remark, does it mean that we can no longer input the point at infinity?
If so, we should say that. If we still can, then why does removing the remark make sense?
Keeping the remark then would not hurt and would be more explicit.
There was a problem hiding this comment.
Ah thanks for bringing this up. I removed this because I did not think it made sense to state anymore. No remark means no restrictions. For example, we do not state that g1_lincomb_naive can be used with points at infinity either.
4 participants
I think we should simplify this & always use Pippenger. This might be a little slower but having a single path is nice.
Also, removes the two remarks which are no longer relevant.
Before
After